-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revamp broadcast IPC #565
base: main
Are you sure you want to change the base?
Revamp broadcast IPC #565
Conversation
- Rename `SocketConnectionFrameReader` to `SocketConnectionSampleReader`. - Utilize the new `HTTPMessageReader` and `BroadcastSampleDecoder` types. - Replaces the functionality of the existing `Message` class. - `didCapture` now emits a `BroadcastSample`. - General refactoring for enhanced readability.
- Uses the Network framework - Enables bi-directional communication and dynamic message headers
- Create `BroadcastUploader` and `BroadcastReceiver` utilizing `IPCChannel` - Separates concerns of socket communication, message format, and sample encoding/decoding - Integrate into existing code - Remove old components
Ensure proper handling of channel closure initiated either by the capture ending or the broadcast ending.
- Fix socket tests for iOS by using relative paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks pretty good, no comments on it from a first pass read-through. i'm going to come back once the other PR lands and you merge it in here and I will test it and then leave more comments if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just a handful of small comments
do { | ||
let receiver = try await BroadcastReceiver(socketPath: socketPath) | ||
logger.debug("Broadcast receiver connected") | ||
self?.receiver = receiver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this have to be done within the task? it would be better if creating the receiver and assigning it to self could be done synchronously within createReceiver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a result of a design decision I made with IPCChannel
, which BroadcastReceiver
uses under the hood. Specifically, I designed IPCChannel
to use the RAII pattern, making its initialization asynchronous so that the consumer always receives an open channel after initialization. This approach also allows IPCChannel
to conform to Sendable
without additional synchronization, as the underlying NWConnection
is both a constant (let) and Sendable
. I am open to feedback about this design.
private let imageContext = CIContext(options: nil) | ||
private let colorSpace = CGColorSpaceCreateDeviceRGB() | ||
|
||
private func jpegEncode(_ imageBuffer: CVImageBuffer) throws -> Data { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we convert to jpeg at all here? why not just dump the sample buffers directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach isn’t ideal, but for this PR I kept the focus solely on IPC. I used the same JPEG encoding method as the original implementation for consistency. In the future, I’d like to explore alternatives—directly dumping the sample buffer would be ideal if the socket bandwidth allows.
This PR revamps inter-process communication between the main app and the broadcast extension, replacing deprecated Core Foundation APIs. Additionally, it enhances testability, enables bi-directional communication, and introduces a more flexible message structure.
Summary of new types:
IPCChannel
: An abstraction for inter-process communication, built on top of the Network framework. Allows asynchronous sending and receiving of messages consisting of a dynamic header (any type conforming toCodable
) and a data payload.BroadcastImageCodec
: Encapsulates functionality for encoding/decoding image samples for transport. For now, this uses the same method of JPEG encoding/decoding as the current implementation.BroadcastUploader
: Sends samples from ReplayKit to the main app, built on top ofIPCChannel
.BroadcastReceiver
: Receives samples from the broadcast extension, built on top ofIPCChannel
.This PR sets the groundwork for adding support for audio samples, but I decided to submit that in a separate PR (#576) as this is a large change (though not breaking).